Skip to content

Welcome page#52

Open
jerembdn wants to merge 9 commits intomasterfrom
jerembdn/welcome
Open

Welcome page#52
jerembdn wants to merge 9 commits intomasterfrom
jerembdn/welcome

Conversation

@jerembdn
Copy link
Copy Markdown
Member

@jerembdn jerembdn commented Nov 25, 2023

No description provided.

@jerembdn jerembdn added enhancement New feature or request good first issue Good for newcomers labels Nov 25, 2023
@jerembdn jerembdn requested a review from antoinekm November 25, 2023 11:30
@jerembdn jerembdn self-assigned this Nov 25, 2023
antoinekm
antoinekm requested changes Nov 25, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@antoinekm antoinekm left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, i believe it could just be better to have a main app, also used to make the dashboard, with react router dom we could have a /welcome route instead of 10 react apps for a page, so it will reduce the extension build size, also adding pretty animations could improve the ux, and please commit using the following guidelines: https://docs.onruntime.com/contributing/commits

src/pages/Welcome/Welcome.tsx
font-size: 48px;
font-weight: 600;
text-align: center;
color: var(--primary);
Copy link
Copy Markdown
Member

@antoinekm antoinekm Nov 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's true that I didn't do it everywhere, but get into the habit of using fallback colors when you use css variables

Copy link
Copy Markdown
Member Author

@jerembdn jerembdn Nov 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk how to do it

Copy link
Copy Markdown
Member

@antoinekm antoinekm Nov 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://defensivecss.dev/tip/css-variable-fallback/

src/pages/Welcome/Welcome.tsx
display: flex;
align-items: center;
justify-content: center;
height: 100vh;
Copy link
Copy Markdown
Member

@antoinekm antoinekm Nov 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use 100vh because we cant scroll if the window height is low

Copy link
Copy Markdown
Member Author

@jerembdn jerembdn Nov 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so i don't know how to do it

src/components/ui/Button/index.tsx
@@ -0,0 +1,94 @@
import React from "react";
Copy link
Copy Markdown
Member

@antoinekm antoinekm Nov 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this file you just dont respect the sizes indicated on the figma

Copy link
Copy Markdown
Member Author

@jerembdn jerembdn Nov 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahbon

src/pages/Popup/index.tsx
import { createRoot } from "react-dom/client";

import Popup from "./Popup";
import "./index.css";
Copy link
Copy Markdown
Member

@antoinekm antoinekm Nov 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove it

@jerembdn
Copy link
Copy Markdown
Member Author

jerembdn commented Nov 25, 2023

ça a vraiment rien à voir avec le dashboard

@antoinekm
Copy link
Copy Markdown
Member

antoinekm commented Nov 25, 2023

yes, it's got nothing to do with the dashboard, but we're not going to make 10 react applications in a single extension, so we might as well combine everything in one application and put the dashboard, welcome and settings in it - the only thing that should be separate is the popup, because we can't do otherwise.

@jerembdn
Copy link
Copy Markdown
Member Author

jerembdn commented Nov 25, 2023

It could be added when the dashboard is made

@antoinekm
Copy link
Copy Markdown
Member

antoinekm commented Nov 27, 2023

Or you can do it now so we can start the dashboard

@antoinekm
Copy link
Copy Markdown
Member

antoinekm commented Dec 2, 2023 •
edited
Loading 

chrome.runtime.onInstalled.addListener((details) => {
  if (details.reason === 'install') {
    // code...
  }
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@antoinekm antoinekm antoinekm requested changes

Assignees

@jerembdn jerembdn

Labels

enhancement New feature or request good first issue Good for newcomers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jerembdn @antoinekm